-
Notifications
You must be signed in to change notification settings - Fork 3k
Implement SEP-990 Enterprise Managed OAuth #1721
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Implement SEP-990 Enterprise Managed OAuth #1721
Conversation
maxisbey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I was missing something,
|
Hi @maxisbey, I have addressed all your comments. Could you please review the PR? |
|
Hi @BinoyOza-okta, I owe you a review on this but won't be able to get to it until Jan 23rd while I wrap up conformance tests for SDK tiering. To make progress in the meantime, a conformance test for this feature would be really helpful to ensure the implementations are compatible across SDKs. Cross-linking: modelcontextprotocol/typescript-sdk#1328 Thanks for your patience! |
…naged Auth support. - Written unit test cases for client and server implementation of the enterprise managed auth code.
…/extensions/enterprise_managed_auth.py 232->235, 304->307. - Resolved pre-commit errors.
…a and JWTBearerGrantRequestData. - Added snippet file for adding code to the README.md file. - Added new section in README.md file to add information regarding: "how to use the access token once you get it" and "How does this work when the client ID is expired?".
…-990) Implements comprehensive conformance testing for enterprise managed authorization flows including RFC 8693 token exchange and RFC 7523 JWT bearer grant with ID-JAG tokens. - Add 3 conformance test scenarios to client.py: * auth/enterprise-id-jag-validation - Validates ID-JAG token structure * auth/enterprise-token-exchange - Tests OIDC ID Token → ID-JAG → Access Token flow * auth/enterprise-saml-exchange - Tests SAML Assertion → ID-JAG → Access Token flow - Create enterprise_auth_server.py: * Implements RFC 8693 token exchange endpoint (/token-exchange) * Implements RFC 7523 JWT bearer grant endpoint (/oauth/token) * Provides OAuth metadata endpoint for discovery * Supports both OIDC ID tokens and SAML assertions * Issues ID-JAG tokens with proper structure (typ: oauth-id-jag+jwt) * Validates bearer tokens and provides protected MCP endpoints - Add run-enterprise-auth-with-server.sh: * Starts mock server on port 3002 * Dynamically fetches test context * Runs all 3 enterprise auth scenarios * Reports detailed test results * Cleans up servers on exit - Update conformance.yml workflow: * Add enterprise-auth-conformance job * Runs on every pull request * Marked as optional (continue-on-error: true) * Tests run in parallel with other conformance checks - Add fastapi>=0.115.0 to dev dependencies * Required for mock server implementation * Only needed for conformance testing * Update uv.lock accordingly - Fix docstring formatting in enterprise_managed_auth_client.py: * Update get_id_token_from_idp() to follow PEP 257 (D212) * Fix multi-line docstring to start summary on first line * Apply fix to both example file and README.md * Ensures all example tests pass - Minor updates to enterprise_managed_auth.py: * Improve error handling * Add validation for token exchange parameters ✅ ID-JAG Token Validation ✅ OIDC ID Token Exchange Flow ✅ SAML Assertion Exchange Flow - RFC 8693 Token Exchange (ID Token and SAML) - RFC 7523 JWT Bearer Grant - ID-JAG token structure validation - OAuth metadata discovery - Bearer token authentication - Error handling and edge cases - SEP-990: Enterprise Managed Authorization - RFC 8693: OAuth 2.0 Token Exchange - RFC 7523: JWT Profile for OAuth 2.0 Client Authentication - RFC 8707: Resource Indicators for OAuth 2.0 Run conformance tests: ```bash ./.github/actions/conformance/run-enterprise-auth-with-server.sh
8935a0f to
b16b652
Compare
|
Hi @pcarleton, |
| elicitation-sep1034-client-defaults - Elicitation with default accept callback | ||
| auth/client-credentials-jwt - Client credentials with private_key_jwt | ||
| auth/client-credentials-basic - Client credentials with client_secret_basic | ||
| auth/enterprise-token-exchange - Enterprise auth with OIDC ID token (SEP-990) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these tests have been consolidated to 1, see modelcontextprotocol/conformance#110 and please update to match
| with: | ||
| node-version: 24 | ||
| - run: uv sync --frozen --all-extras --package mcp | ||
| - run: ./.github/actions/conformance/run-enterprise-auth-with-server.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should not be necessary as the all command above should cover it.
| @@ -0,0 +1,332 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a standalone conformance test? Sorry if I didn't explain clearly, I meant in the conformance repo. See the link modelcontextprotocol/conformance#110 we want this to pass the tests in that repo so we know the different SDK's are implementing it similarly
| await session.initialize() | ||
|
|
||
| # Call tools with automatic retry on token expiration | ||
| result = await call_tool_with_retry( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we check the token is expired at 142, then we call with retry here. It seems like one or the other would be better. both seems redundant.
| print(f"Resource: {resource.uri}") | ||
|
|
||
|
|
||
| async def maintain_active_session( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this function for? it doesn't seem used. suggest removing it.
|
|
||
| # For now, raise NotImplementedError as this requires integration | ||
| # with the full httpx auth flow | ||
| raise NotImplementedError( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we call those methods here? without them, you can't drop this in as an OAuthClientProvider
| Decoded ID-JAG claims | ||
|
|
||
| Note: | ||
| For verification, use server-side validation instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's reword this, saying "This function does not verify the JWT, instead relying on the receiving server to validate it."
| """Check if the access token has expired.""" | ||
| if access_token.expires_in: | ||
| # Calculate expiration time | ||
| issued_at = datetime.now(timezone.utc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the purpose of this method? it's always going to be false because.
| storage: TokenStorage, | ||
| idp_token_endpoint: str, | ||
| token_exchange_params: TokenExchangeParameters, | ||
| redirect_handler: Callable[[str], Awaitable[None]] | None = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this class need a redirect handler and callback handler? since it's using ID-JAG there shouldn't be a redirect or callback
| client_metadata: OAuthClientMetadata, | ||
| storage: TokenStorage, | ||
| idp_token_endpoint: str, | ||
| token_exchange_params: TokenExchangeParameters, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it's a bit awkward to take in TokenExchangeParameters here.
I think id_token_provider: Callable[[], Awaitable[str]], would be a good argument.
we also likely need idp_client_id / idp_client_secret as arguments.
I think it'd be good to rely on the existing mechanisms to get the resource URL and AS metadata, e.g.:
async def _perform_authorization(self) -> httpx.Request:
audience = str(self.context.oauth_metadata.issuer)
resource = self.context.get_resource_url()
id_token = await self.id_token_provider()
# Side request to IdP for token exchange (RFC 8693)
id_jag = await self._exchange_for_id_jag(id_token, audience, resource)
# Return JWT bearer grant request (RFC 7523) for base class to send
return self._build_jwt_bearer_request(id_jag)
| "dirty-equals>=0.9.0", | ||
| "coverage[toml]>=7.10.7,<=7.13", | ||
| "pillow>=12.0", | ||
| "fastapi>=0.115.0", # For enterprise auth conformance test mock server |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not write the test without FastAPI? Seems like a heavy import for a test.
|
This conformance tests is very cute in theory, but it's adding too much noise to the repository. On another unrelated note, this kind of PR makes me extremely demotivated to continue to help this project. It's a 2.8k LOC, which none of the other maintainers seems to care. I keep seeing auth related code, PRs, commits with bad coding practices, and being accepted just for the fear of the word "security", "compliance", "enterprise". |
This PR implements the client-side components of SEP-990: Enterprise Managed Authorization. It introduces the
EnterpriseAuthOAuthClientProviderto handle the full token exchange flow required for Enterprise SSO, including RFC 8693 (Token Exchange) and RFC 7523 (JWT Bearer Grant).Motivation and Context
Implements: SEP-990
To support enterprise environments where direct API keys are not compliant, the Python SDK needs to support "Managed Authorization." This implementation allows the SDK to:
This aligns the Python SDK with the architecture defined in the SEP-990 implementation guide.
Implementation Details
The following components have been added to
src/mcp/client/auth/extensions/enterprise_managed_auth.py:TokenExchangeParametersandTokenExchangeResponseusing Pydantic to strictly type the exchange payloads.EnterpriseAuthOAuthClientProvider, which extends the baseOAuthClientProviderto orchestrate the exchange logic.urn:ietf:params:oauth:token-type:id-jagtoken types.How Has This Been Tested?
I have implemented comprehensive unit tests in
tests/client/auth/test_enterprise_managed_auth_client.pyusingpytestandunittest.mock.The testing suite covers the following scenarios:
Data Model Validation:
TokenExchangeParameterscorrectly generates requests for both OIDC ID Tokens (test_token_exchange_params_from_id_token) and SAML Assertions (test_token_exchange_params_from_saml_assertion).RFC 8693 Token Exchange Logic:
httpxto verify the correct payload structure (grant types, token types) is sent to the IdP.client_idandclient_secretare correctly injected into the request body when configured (test_exchange_token_with_client_authentication).RFC 7523 JWT Bearer Grant Logic:
Network Edge Cases:
httpx.ConnectError,httpx.ReadTimeout) to ensureOAuthTokenErroris raised with descriptive messages.Breaking Changes
No.
This is an additive extension. The core
OAuthClientProviderremains backward compatible. Only users specifically importing and usingEnterpriseAuthOAuthClientProviderwill be affected.Types of changes
Checklist
Additional context
pydanticfor model validation andhttpxfor async requests.src/mcp/client/auth/extensions/).